[rtsan] Handle attributed IR function declarations #169577
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-compiler-rt-sanitizer Author: None (davidtrevelyan) ChangesAddresses #169377. Previously, the RealtimeSanitizer pass only handled attributed function definitions in IR, and attributed function declarations caused it to crash. To fix the issue, we must check whether the IR function is empty before attempting to do any manipulation of its instructions. This PR:
Full diff: https://github.com/llvm/llvm-project/pull/169577.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
index 5ef6ffb58a7c1..adb0e518c6074 100644
--- a/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
@@ -61,23 +61,21 @@ static void insertCallAtAllFunctionExitPoints(Function &Fn,
insertCallBeforeInstruction(Fn, I, InsertFnName, FunctionArgs);
}
-static PreservedAnalyses rtsanPreservedCFGAnalyses() {
- PreservedAnalyses PA;
- PA.preserveSet<CFGAnalyses>();
- return PA;
-}
+static void runSanitizeRealtime(Function &Fn) {
+ if (Fn.empty())
+ return;
-static PreservedAnalyses runSanitizeRealtime(Function &Fn) {
insertCallAtFunctionEntryPoint(Fn, "__rtsan_realtime_enter", {});
insertCallAtAllFunctionExitPoints(Fn, "__rtsan_realtime_exit", {});
- return rtsanPreservedCFGAnalyses();
}
-static PreservedAnalyses runSanitizeRealtimeBlocking(Function &Fn) {
+static void runSanitizeRealtimeBlocking(Function &Fn) {
+ if (Fn.empty())
+ return;
+
IRBuilder<> Builder(&Fn.front().front());
Value *Name = Builder.CreateGlobalString(demangle(Fn.getName()));
insertCallAtFunctionEntryPoint(Fn, "__rtsan_notify_blocking_call", {Name});
- return rtsanPreservedCFGAnalyses();
}
PreservedAnalyses RealtimeSanitizerPass::run(Module &M,
diff --git a/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll b/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll
new file mode 100644
index 0000000000000..a5336b9e9e491
--- /dev/null
+++ b/llvm/test/Instrumentation/RealtimeSanitizer/rtsan_attrib_declare.ll
@@ -0,0 +1,12 @@
+; RUN: opt < %s -passes='rtsan' -S | FileCheck %s
+
+declare void @declared_realtime_function() sanitize_realtime #0
+
+declare void @declared_blocking_function() sanitize_realtime_blocking #0
+
+; RealtimeSanitizer pass should ignore attributed functions that are just declarations
+; CHECK: declared_realtime_function
+; CHECK-EMPTY:
+; CHECK: declared_blocking_function
+; CHECK-EMPTY:
+
|
Addresses llvm#169377. Previously, the RealtimeSanitizer pass only handled attributed function definitions in IR, and attributed function declarations caused it to crash. To fix the issue, we must check whether the IR function is empty before attempting to do any manipulation of its instructions.
davidtrevelyan
force-pushed
the
rtsan/handle-attributed-ir-declarations
branch
from
8877924 to
36ac559
Compare
cjappl
left a comment
cjappl
left a comment
There was a problem hiding this comment.
Seems reasonable enough, just have some questions before I approve
| if (Fn.empty()) | ||
| return; | ||
|
|
||
| static PreservedAnalyses runSanitizeRealtime(Function &Fn) { |
There was a problem hiding this comment.
Is not returning these preserved analyses going to bite us elsewhere? I remember most of the sanitizer transform passes (or transform passes generally) returning these.
There was a problem hiding this comment.
Perhaps! But the issue is we're already not returning these preserved analyses - they're sadly discarded at the moment. I would advocate for this NFC because it makes the current behaviour explicit - even if it's not correct. I bundled this change in with this PR because it made the implementation of the checking a bit easier, but I'd be happy to submit this in a separate preparatory PR first - if that's easier to follow - just let me know.
| } | ||
| static void runSanitizeRealtime(Function &Fn) { | ||
| if (Fn.empty()) | ||
| return; |
There was a problem hiding this comment.
Is there precedence in other sanitizer passes for doing this? (Curious how they solved this problem)
There was a problem hiding this comment.
Thanks for the good idea. I hadn't looked before drafting this PR, but it appears like ASan solves it a similar way at one level higher - at the top level of the pass's run method:
ThreadSanitizer has a bool check for whether it should sanitizeFunction, which returns false if there are no calls in it. MemorySanitizer does the same thing as AddressSanitizer with a continue at the top level if the function is .empty().
If you prefer the ASan/MSan semantics I'm very happy to switch to it 👍
|
Looks good! doesn't crash anymore and the rtsan tests i added to rustc also still pass. |
You can use this format. |
cjappl
left a comment
cjappl
left a comment
There was a problem hiding this comment.
Approved as written! I lean slightly to breaking the NFC into another commit but not strongly enough to block based on it
Thanks for investigating the other sanitizers. Looks like there is good precedence for this type of check
5 participants
Addresses #169377.
Previously, the RealtimeSanitizer pass only handled attributed function definitions in IR, and we have recently found that attributed function declarations caused it to crash. To fix the issue, we must check whether the IR function is empty before attempting to do any manipulation of its instructions.
This PR:
Functions areempty()in each relevant RTSan pass routinertsanPreservedCFGAnalysesfrom the pass, whose result was unused and which would otherwise have complicated the fix